Provide default values information in generated config file, better handle logging default parameters #3053

glemercier · 2019-09-22T16:14:08Z

Improvements linked to request #3002

This pull request provides information on default values for each config parameter as comments of the auto-generated grin-server.toml file, as per requested in issue mentioned above.
It also better handles cases where some logging parameters are not provided, leading to the program exiting. Instead we use optional parameters set to their default value when absent from the config file.

This PR improves overall configuration experience based on @josef-v feedback but it could be amended to better improve simple configuration inconsistencies coming from anyone else's request.

antiochp

Hi! Thanks for taking a look at this.

I'm not sure this actually solves the underlying problem though.

#3002 is about having less config file and relying on defaults. Whereas these changes actually increases the size of the generated config file.

These additional lines in the config file are an additional thing to keep updated as config changes and evolves over time.

Ideally all this config, specifically the default config would be sourced from one single "source of truth".
For example if we make the decision to change the default port -

#the address on which services will listen, e.g. Transaction Pool
#Default: 127.0.0.1:3413

We would have to remember to change the text in this "Default" line in the config file comments.

I suspect #3002 would be solved by taking a look at how we currently handle default values and taking better advantage of "serde defaults". We are inconsistent in how we handle this in different parts of the code.

The way we should be handling defaults (and non-required values in the config file itself) -

grin/pool/src/types.rs

Lines 51 to 54 in 1c072f5

    
           pub struct DandelionConfig { 
        
           	/// Length of each "epoch". 
        
           	#[serde(default = "default_dandelion_epoch_secs")] 
        
           	pub epoch_secs: u16,

This defines a config entry and defines a fn to call if the value is not present in the config file.
This removes the need to wrap anything in an Option<> and we don't need special case handling of None if its missing.

And then in comments.rs we can reuse this same fn to generate a "default" comment line describing what the default value is. Ideally as a commented out config line that can be easily un-commented out.

glemercier · 2019-09-23T10:33:07Z

Ok I get your point, fair request indeed. I can confirm "serde defaults" are not used everywhere (mostly in one place if I remember well). I'll look into improving this first.

I don't think @josef-v was concerned with the size of the default configuration file as long as he can trim it himself without breaking things. If you think there is a better place to extract and expose the default config values, I'll be happy to look into it.

josef-v · 2019-09-23T11:27:28Z

#3002 is about having less config file and relying on defaults. Whereas these changes actually increases the size of the generated config file.

I agree with @glemercier that adding another comment is not an issue as you can simply delete it later without any consequences. So having defaults in a comment is ok, maybe unnecessary as we can have the config file generated with defaults and some statement "config file generated with default values" on top of it. However hardcoding these values into config is not a way to go.

Ideally all this config, specifically the default config would be sourced from one single "source of truth".

That's exactly what I meant.

glemercier · 2019-09-23T11:55:59Z

Thank you both for your feedback and implementation details. I'll work on this and propose an update to this PR.

glemercier · 2019-10-02T20:59:30Z

Update to the PR:

Use serde defaults on all the config structures. Removed Option<> where relevant for cleaner code
Put all the default values as constants a the top of concerned files. They may be moved to a single dedicated file for a central default config place
Explicitely implement Serialize/Deserialize for Capabilities bitflags
This has for effect that the parameter appears like this in the config file:

[server.p2p_config]
capabilities = 15

instead of previously

[server.p2p_config.capabilities]
bits = 15

however it is possible to comment the line in the new case and it will take the default value, which was not possible the old way (using derived Serialize/Deserialize)

When generating default config, commenting out all values, since default will be used
An (almost) empty config file will work, only the root titles are required to avoid crashing at startup:

[server]
[logging]

Feel free to provide feedback on this PR and suggest any change.

antiochp · 2019-10-12T08:12:23Z

Planning to take a closer look over this over the next couple of days. But 👍 on the approach.

phyro · 2021-02-12T08:24:34Z

@antiochp this one has accumulated quite a bit of conflicts. Is the proposed change still relevant?

yeastplume · 2022-01-07T13:12:12Z

Closed and marked as 'abandoned'. This does not mean the PR is unworthy. This means that merging it would require further work that nobody appears prepared to do at this time, or the original author has been unresponsive for a significant amount of time. Anyone who want to continue work on this functionality is free to re-open this at any time.

antiochp requested changes Sep 23, 2019

View reviewed changes

glemercier force-pushed the master branch from 063a9cd to 516f504 Compare October 2, 2019 20:48

glemercier added 9 commits October 5, 2019 15:55

fix processing of config lines that contain array values

9f8236a

implement Serialize/Deserialize for Capabilities instead of deriving

6994e69

handle default values for P2P config types

8a72834

handle default values for servers config types

584956f

handle default values for pool config types

4fc576b

handle default values for util config types

45fd636

adapt generated config file comments to new default values

9256f3f

comment default values (except paths) in generated config files

eab3b93

rustfmt

d8a8d62

glemercier force-pushed the master branch from 516f504 to d8a8d62 Compare October 5, 2019 13:59

glemercier requested a review from antiochp October 11, 2019 12:12

quentinlesceller added review needed and removed review needed labels Jan 21, 2020

phyro added the abandoned Abandoned due to either lack of review or contribution. Could get revisited later. label Jan 6, 2022

yeastplume closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide default values information in generated config file, better handle logging default parameters #3053

Provide default values information in generated config file, better handle logging default parameters #3053

glemercier commented Sep 22, 2019

antiochp left a comment •

edited

Loading

glemercier commented Sep 23, 2019

josef-v commented Sep 23, 2019

glemercier commented Sep 23, 2019

glemercier commented Oct 2, 2019

antiochp commented Oct 12, 2019

phyro commented Feb 12, 2021

yeastplume commented Jan 7, 2022

	pub struct DandelionConfig {
	/// Length of each "epoch".
	#[serde(default = "default_dandelion_epoch_secs")]
	pub epoch_secs: u16,

Provide default values information in generated config file, better handle logging default parameters #3053

Provide default values information in generated config file, better handle logging default parameters #3053

Conversation

glemercier commented Sep 22, 2019

antiochp left a comment • edited Loading

Choose a reason for hiding this comment

glemercier commented Sep 23, 2019

josef-v commented Sep 23, 2019

glemercier commented Sep 23, 2019

glemercier commented Oct 2, 2019

antiochp commented Oct 12, 2019

phyro commented Feb 12, 2021

yeastplume commented Jan 7, 2022

antiochp left a comment •

edited

Loading